Changes to allow backward compatibility with plain ole CodeMirror themes #8673
Conversation
Changes to allow backward compatibility with plain ole CodeMirror themes
|
Thanks for the fix! |
|
|
||
| this.file = file; | ||
| this.name = options.name || (options.title || fileName).toLocaleLowerCase().replace(/[\W]/g, '-'); | ||
| this.name = options.name || (options.title || fileName.replace(/.[\w]+$/gi, '')).toLocaleLowerCase().replace(/[\W]/g, '-'); |
There was a problem hiding this comment.
Well, didn't you mean replace(/\.[\w]+$/gi, '') (escaped dot)?
You don't need the i option, btw.
There was a problem hiding this comment.
Ah, and what about double extensions like .css.min?
We should maybe add an unit test for that, too.
There was a problem hiding this comment.
I had escaped initially, but when I tested it it didn't make a difference. Check it out http://regex101.com/r/iI6oQ0/1
There was a problem hiding this comment.
Err, yeah, escaping the dot is required because "indexhtml" matches the regex as it stands now.
There was a problem hiding this comment.
Of course, these are edge cases for things that aren't hit in core usage of the feature anyhow...
There was a problem hiding this comment.
I want to make sure the stuff works as expected though so I am make another PR with the dot escaped. Good catch guys!
Loading theme files directly instead of a package breaks old themes; e.g. vanilla codemirror themes. The issue is caused by adding the file extension as part of the theme name. This logic was to handle edge cases when themes had the same filename but different extensions, but this edge case should really be resolved by giving files a better name. This was important in earlier impls of themes that provided an interface to load all the theme files in a diretory (loadDirectory) but that interface was removed a while ago.